Add ResourceLimiterAsync, which can yield until resource is available #3393
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
fuzzing
Subscribe to Label Actioncc @fitzgen, @peterhuene
This issue or pull request has been labeled: "fuzzing", "wasmtime:api"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
pchickey
force-pushed
the
pch/async_limiting
branch
2 times, most recently
from
1e14251
to
7d0611b
Compare
There was a problem hiding this comment.
Looking good to me! If you're up for it I think it would also be interesting to have tests which panic! in the memory growth (either in the async or sync version) from the custom trait implementation which then asserts that the panic can be caught from the other side of wasm and handled.
added 10 commits
October 21, 2021 12:10
pchickey
force-pushed
the
pch/async_limiting
branch
from
edd3b8e
to
d3deaae
Compare
added 6 commits
October 21, 2021 15:07
idk why this didnt work in the old factoring! but im glad it does
pchickey
changed the title
There was a problem hiding this comment.
All looks good to me! A few minor things I might also ask for:
- Doc updates to the blocking methods that they can panic for async stores with an async limiter configured
- Could you add a test where the async-ness is exercised as well? I noticed most of them using the async trait were just async conditions, but could there be one which also sleeps or synchronizes with some other task or something like that?
added 2 commits
October 22, 2021 11:44
alexcrichton
approved these changes
Oct 22, 2021
alexcrichton
approved these changes
Oct 25, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR:
ResourceLimiter's definition fromwasmtime_runtimetowasmtimeResourceLimiterAsync, which is just like aResourceLimiterexcept thememory_growingandtable_growingmethods areasync(via#[async_trait]). This is so that async embeddings of wasmtime can handle memory or table growing byawaiting for a resource to be available.ResourceLimiterAsyncmethods to await inside memory & table grows, we need to add async variants ofMemory::new,Memory::grow,Table::new, andTable::grow. These variants are not required for all asyncStores, only for stores which are given aStore::limiter_async. So, this won't break any existing code.ResourceLimitertraits now gettable_grow_failed, which corresponds tomemory_grow_failed. Better to just get this over with now than have to add it in later.I had to restructure a fair bit of the interface between runtime and wasmtime for this PR, including extending the
wasmtime_runtime::Storetrait, and changing the error type passed between them toanyhow::Errorinstead ofBox<dyn std::error::Error + Send + Sync>.Additionally, both
ResourceLimiters are now safer: wasmtime_runtime now catches any panics that occur from the user's impl of the traits.